crypto: expose new method OlmMachine::try_decrypt_room_event
#4116
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4116 +/- ##
==========================================
- Coverage 84.69% 84.57% -0.13%
==========================================
Files 269 269
Lines 28816 28825 +9
==========================================
- Hits 24405 24378 -27
- Misses 4411 4447 +36 ☔ View full report in Codecov by Sentry. |
richvdh
force-pushed
the
rav/try_decrypt_room_event
branch
from
7420e03
to
33f5dd9
Compare
richvdh
force-pushed
the
rav/try_decrypt_room_event
branch
from
33f5dd9
to
61f3cec
Compare
richvdh
requested review from
bnjbvr and
andybalaam
and removed request for
a team
| /// that session for some reason (e.g. incorrect MAC). | ||
| /// | ||
| /// This represents all `vodozemac::megolm::DecryptionError`s, with the | ||
| /// exception of `UnknownMessageIndex`, which is represented as |
There was a problem hiding this comment.
I found this difficult to parse because of the special meaning of "exception" in my head. Would "with the exception of" -> "except" be better?
| fn unknown_utd_reason() -> UnableToDecryptReason { | ||
| UnableToDecryptReason::Unknown | ||
| } |
There was a problem hiding this comment.
For what it's worth, you can instead derive(Default) to UnableToDecryptReason, and then mark one variant as the default:
#[default]
#[doc(hidden)]
Unknown,There was a problem hiding this comment.
Yeah, I wanted to explicitly avoid implementing Default for UnableToDecryptReason, because then people might start using it outside the deserialization case.
| MegolmDecryptionFailure, | ||
|
|
||
| /// The event could not be deserialized after decryption. | ||
| PayloadDeserializationFailure, |
There was a problem hiding this comment.
double nit: since those are errors, i'd say "failure" in the variant's name is redundant, and would lightly encourage to remove it.
There was a problem hiding this comment.
I don't really agree. If I compare with something like MissingMegolmSession: a missing megolm session is a sensible reason for being unable to decrypt an event. So is a "megolm decryption failure". But just MegolmDecryption on its own doesn't really make sense to me.
I'm prepared to believe there could be better names for these things (UnableToDecryptMegolmMessage? VodozemacDecryptionError?), but I don't think MegolmDecryption would be a good one.
| @@ -2540,6 +2536,38 @@ pub struct EncryptionSyncChanges<'a> { | |||
| pub next_batch_token: Option<String>, | |||
| } | |||
|
|
|||
| fn megolm_error_to_utd_info( | |||
There was a problem hiding this comment.
Can you write a tiny doc comment for this, please? (It wasn't clear at a first glance at the call-site why this could return an error...)
| Some(UnsignedDecryptionResult::UnableToDecrypt(UnableToDecryptInfo { | ||
| session_id, | ||
| })) | ||
| Err(e) => { |
There was a problem hiding this comment.
nit: can you name this variable err, if not error, please? One-letter variables are bleh.
| ) -> Result<RoomEventDecryptionResult, CryptoStoreError> { | ||
| match self.decrypt_room_event_inner(raw_event, room_id, true, decryption_settings).await { | ||
| Ok(decrypted) => Ok(RoomEventDecryptionResult::Decrypted(decrypted)), | ||
| Err(e) => Ok(RoomEventDecryptionResult::UnableToDecrypt(megolm_error_to_utd_info( |
| let decrypt_result = | ||
| bob.try_decrypt_room_event(&room_event, room_id, &decryption_settings).await.unwrap(); | ||
| assert_let!(RoomEventDecryptionResult::UnableToDecrypt(utd_info) = decrypt_result); | ||
| assert!(utd_info.session_id.is_some()); |
Add a field to store the reason that the decryption failed
richvdh
force-pushed
the
rav/try_decrypt_room_event
branch
from
390f187
to
f003f6b
Compare
... which returns an enum which encodes decryption failure information, rather than returning
MegolmErrorTo make this work, we also extend the existing
UnableToDecryptInfoto include a reason code for the failure.Part of #4048